-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the CharSequence wrappers in Predef non-implicit, for JDK 15 #9292
Make the CharSequence wrappers in Predef non-implicit, for JDK 15 #9292
Conversation
In JDK 15 CharSequence has an isEmpty method with a default implementation, which clashes with our Array[Char]#isEmpty, IndexedSeq[Char]#isEmpty, as well as our StringBuilder and reflect's Name.
isEmpty is defined directly on Seq, so I don't think that an implicit conversion also defining isEmpty leads to a clash. |
I added it mostly to get rid of the other way CharSequence, which we don't control, can break us. |
do we want to deprecate these and suggest people write
in the |
Bit of a mixed message with http://dotty.epfl.ch/docs/reference/other-new-features/creator-applications.html :)
Yep that's what I suggested in the previous PR too. |
Just I agree to deprecating |
community build results for 2.13.4-bin-0230f94-SNAPSHOT are in. my conclusion: this stuff is rarely used. it is okay to go ahead with this. details follow:
scala-parser-combinators needed changes to 3 call sites in 3 different source files: scala/scala-parser-combinators#306 — it is using scalatest was just using I had to fix those (and, separately, scalatest-3-0) in order to get a usable run. The remaining failures are: the twirl failure is actually about that the exhaustivity changes and not about this. so that leaves scalafix and scoverage in scalafix, four call sites in a single source file is affected; in scoverage, two call sites in a single source file this all seems well within the range of acceptability to me |
One thing that I don't think has been discussed: should this be backported to 2.12? Or perhaps the original idea of hacking implicit search would be more appropriate there? Or Java 15+ should just be declared unsupported? (Might be problematic given that Java 17 is not that far away and will be an LTS release). |
Yeah, we'll want to backport this to 2.12, though it's not as critical as 2.13.4 for it to be in 2.12.13, IMO. PR's welcome. |
I agree.
A good replacement would be
That wasn't done in the end, it seems? |
… JDK 15 In JDK 15 CharSequence has an isEmpty method with a default implementation, which clashes with our Array[Char]#isEmpty, IndexedSeq[Char]#isEmpty, as well as our StringBuilder and reflect's Name. Backport of scala#9292 from 2.13.x to 2.12.x
Yes, it's supported. I even already used it as a replacement in one of our tests that was relying on |
as per discussion on scala#9292
2.13.4 brings JDK 15 support with scala/scala#9292
2.13.4 brings JDK 15 support with scala/scala#9292
2.13.4 brings JDK 15 support with scala/scala#9292 - tests/run/t8153.scala was removed like it was removed in Scala 2 with scala/scala@8f6e522 - tests/neg/missing-implicit-2.check was updated because the implicitNotFound message was changed in 2.13.4 - tests/run/t6260-delambdafy.scala had to be changed to behave the same under JDK <= 15 and JDK 15.
2.13.4 brings JDK 15 support with scala/scala#9292 Also upgrade Scala.js to 1.3.1 so that the testcases we import from it compile with JDK 15. - tests/run/t8153.scala was removed like it was removed in Scala 2 with scala/scala@8f6e522 - tests/neg/missing-implicit-2.check was updated because the implicitNotFound message was changed in 2.13.4 - tests/run/t6260-delambdafy.scala had to be changed to behave the same under JDK <= 15 and JDK 15.
This requires a single source code change to handle scala/scala#9292.
2.13.4 brings JDK 15 support with scala/scala#9292 - tests/run/t8153.scala was removed like it was removed in Scala 2 with scala/scala@8f6e522 - tests/neg/missing-implicit-2.check was updated because the implicitNotFound message was changed in 2.13.4 - tests/run/t6260-delambdafy.scala had to be changed to behave the same under JDK <= 15 and JDK 15.
2.13.4 brings JDK 15 support with scala/scala#9292 - tests/run/t8153.scala was removed like it was removed in Scala 2 with scala/scala@8f6e522 - tests/neg/missing-implicit-2.check was updated because the implicitNotFound message was changed in 2.13.4 - tests/run/t6260-delambdafy.scala had to be changed to behave the same under JDK <= 15 and JDK 15.
2.13.4 brings JDK 15 support with scala/scala#9292 - tests/run/t8153.scala was removed like it was removed in Scala 2 with scala/scala@8f6e522 - tests/neg/missing-implicit-2.check was updated because the implicitNotFound message was changed in 2.13.4 - tests/run/t6260-delambdafy.scala had to be changed to behave the same under JDK <= 15 and JDK 15.
2.13.4 brings JDK 15 support with scala/scala#9292 - tests/run/t8153.scala was removed like it was removed in Scala 2 with scala/scala@8f6e522 - tests/neg/missing-implicit-2.check was updated because the implicitNotFound message was changed in 2.13.4 - tests/run/t6260-delambdafy.scala had to be changed to behave the same under JDK <= 15 and JDK 15.
2.13.4 brings JDK 15 support with scala/scala#9292 - tests/run/t8153.scala was removed like it was removed in Scala 2 with scala/scala@8f6e522 - tests/neg/missing-implicit-2.check was updated because the implicitNotFound message was changed in 2.13.4 - tests/run/t6260-delambdafy.scala had to be changed to behave the same under JDK <= 15 and JDK 15.
Another way the source incompatibility can be experienced, for the record.
|
Problem ======= Upgrade source to https://github.com/scala/scala/releases/tag/v2.12.13 from https://github.com/scala/scala/releases/tag/v2.12.12. This upgrade includes a highlighted feature of configurable warnings and errors (https://www.scala-lang.org/2021/01/12/configuring-and-suppressing-warnings.html / scala/scala#9248). Other noticable changes our code based will experience * Exhaustivity warnings for patterns involving tuples * Better type checking for `CharSequence` arguments scala/scala#9292 * Simplified Reporters scala/scala#8338 * Promotion of `-deprecation` to `-Xlint:deprecation` scala/scala#7714 * Improves performance of building `immutable.{TreeMap,TreeSet}` by using mutation within the builder scala/scala#8794 Full list of changes can be found at https://github.com/scala/scala/pulls?q=is%3Amerged+milestone%3A2.12.13+ Solution ======== * Bump `BUILD` file `SCALA_REV` && `SCALAC_REV_FOR_DEPS`. * Depdnencies which are not built for scala 2.12.13 needed to be bumped `SEMANTICDB_PLUGIN_REV` && `SEMANTICDB_REV` && `SCALAFIX_REV`. * Include `-S-Xlint:-deprecation` to `pants.ini` preventing build failures on deprecated annotations (existing behavior) * Bump `cache_key_gen_version` in `pants.ini` for newly built artifacts on different scala version. * Removed scalafix plugin (`scalac-profiling`) which is not built for 2.12.13. `scalac-profiling` code looks abandoned by ~3 years. * Updated all failing tests that could have depended or expected ordered sequences when the sequence was generated from non ordered collections. Notes ===== It seems a few tests and golden files in source have depended or tested against a stable sort order when sorted order is not guaranteed. This has been seen in a few places such as output json objects (map key fields), code that uses `groupBy` for rekeying results to the user and transforming into sequences, strings built from sequences or maps that are not guaranteed to be ordered. The following PR are related to this change in expectations scala/scala#8794 scala/scala#8948 scala/scala#9218 scala/scala#9376 scala/scala#9091 scala/scala#9216 We took the liberty updating tests with what the current map order would be or updating the test in a way that wouldn't depend on order. Since we may not fully understand all the context of the tests we are hoping this either signals to the owners that there might be some issue with assumed order or signal that upgrading might break implementations due to bug in scala 2.12.13. {D611202} will improve the files changed for workflow builder outside of this change. Please feel to reach out directly and discuss the changes here or bring up issues with this upgrade. Slack [[https://app.slack.com/client/T86S8GHEG/C01NZAFRLFK|#scala-upgrade-2-12-13]] JIRA Issues: SCALA-25 Differential Revision: https://phabricator.twitter.biz/D607826
This change is binary compatible, but not source compatible. Users will need to update callsites to explicitly convert to
CharSequence
, since the conversions are no longer implicit.ArrayCharSequence
is no longer needed and will be deprecated in 2.12.13 and 2.13.5. Instead, usejava.nio.CharBuffer.wrap
to convert anArray[Char]
to aCharSequence
.SeqCharSequence
remains useful, but now must be explicitly constructed.Why was this necessary? In JDK 15 CharSequence has an isEmpty method with a default implementation, which clashes with our Array[Char]#isEmpty, IndexedSeq[Char]#isEmpty, as well as our StringBuilder and reflect's Name.
Fixes scala/bug#12172